feat: add high-performance HNSW RaBitQ index - #1798
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CLiqing The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@CLiqing 🔍 Important: PR Classification Needed! For efficient project management and a seamless review process, it's essential to classify your PR correctly. Here's how:
For any PR outside the kind/improvement category, ensure you link to the associated issue using the format: “issue: #”. Thanks for your efforts and contribution to the community!. |
e778c24 to
aae6c99
Compare
|
/hold |
|
@CLiqing just in case, is this PR compatible with facebookresearch/faiss#5526 ? |
abfc167 to
a9c7389
Compare
|
@alexanderguzhva It is not index-format or runtime-behavior compatible with #5526 because the construction and search contracts differ. This PR builds the graph with exact FP32 distances and then attaches The implementations can coexist in source: the Knowhere type is under |
a9c7389 to
89f7b5d
Compare
| // Store one complete scalar code per dimension in a dense n-bit stream. | ||
| // This combines the sign bit and extra bits at build time while retaining | ||
| // the exact nbits/dimension budget. For nbits=8 this is one byte/dimension. | ||
| bool dense_layout = false; |
There was a problem hiding this comment.
RaBitQuantizer::dense_layout (RaBitQuantizer.h:53) changes the on-disk code layout but is not part of the standard wire format: index_write.cpp:1027 tags every multi-bit IndexRaBitQ as Ixrr and index_read.cpp:2856 default-constructs the index, so a faiss::write_index/read_index round-trip restores dense_layout=false and reinterprets dense codes with packed offsets. read_RaBitQuantizer does not recompute code_size, and for any d % 8 == 0 (64, 128, 768, 1536) the dense and packed sizes are byte-identical, so nothing rejects the file — every distance is silently wrong. IndexRaBitQFastScan::IndexRaBitQFastScan(const IndexRaBitQ&, int) (IndexRaBitQFastScan.cpp:106) copies the quantizer and then reads sign bits at orig_code[j/8] >> (j%8), which is garbage for dense codes, also without a guard.
There was a problem hiding this comment.
Fixed. Dense multi-bit IndexRaBitQ now uses the distinct Ixrd tag; the standard reader restores dense_layout, recomputes and validates code_size, and validates the code buffer length. Converting a dense index to IndexRaBitQFastScan now throws explicitly. Cross-reader round-trip tests cover the standard and cppcontrib readers.
| REQUIRE(index.Build(train, json) == knowhere::Status::success); | ||
| REQUIRE(index.Count() == kNb); | ||
|
|
||
| const auto before = index.Search(query, json, nullptr); |
There was a problem hiding this comment.
Every assertion in test_hnsw_rabitq.cc is self-referential: CheckValidKnnResult (:65) only checks id range and isfinite, CheckKnnOrder (:78) only checks monotonicity, the serialization round-trip (:274) compares the index against itself, CalcDistByIDs (:253) compares two paths that resolve to the same distance computer, and the dense-vs-packed case (:282) compares two layouts of the same algorithm. If the query were rotated with a different matrix than the base vectors, or the shared RaBitQ distance formula had a sign error, all five TEST_CASEs still pass and the index ships returning wrong neighbors. HNSW_RABITQ appears in no other test file, so this suite is the only coverage.
There was a problem hiding this comment.
Fixed. The test now computes an independent FP32 brute-force oracle and checks recall for L2/IP, RBQ1/RBQ4/RBQ8, and aligned/unaligned dimensions. It also verifies COSINE storage/refine distances against original vectors, so rotation, sign, or metric mistakes are no longer self-validating.
| } | ||
| } | ||
|
|
||
| template <SIMDLevel SL> |
There was a problem hiding this comment.
tests/faiss/CMakeLists.txt builds faiss_tests from a hardcoded FAISS_TEST_SRCS list that does not contain test_rabitq_simd.cpp, and this PR does not add it, so the 174 lines of new AVX2/AVX-512 comparison tests never run in knowhere CI. The only remaining coverage for the ~1400 new kernel lines is the dense-vs-packed case in test_hnsw_rabitq.cc:282, which exercises exactly one runtime-detected SIMD level: on an AVX-512 CI host the AVX2 dense kernels are never executed, yet they are the ones that run on AVX2-only production hosts, where a kernel bug produces wrong distances undetected.
There was a problem hiding this comment.
Fixed. test_rabitq_simd.cpp is now listed in tests/faiss/CMakeLists.txt. I built faiss_tests locally and ran 10 RaBitQ tests, including explicit AVX2/AVX-512 single and batch-4 equivalence and the non-BMI2 fallback.
|
|
||
| IndexHNSWRaBitQ::IndexHNSWRaBitQ() = default; | ||
|
|
||
| IndexHNSWRaBitQ::IndexHNSWRaBitQ(faiss::IndexPreTransform* storage_in, int M) |
There was a problem hiding this comment.
IndexHNSWRaBitQ(IndexPreTransform*, int) delegates to IndexHNSW(storage, M) (IndexHNSW.cpp:241), which only builds an empty hnsw(M); entry_point stays -1 so HNSW.cpp:1006 returns immediately and every search yields no results, while the constructor still sets ntotal from the storage and presents the index as populated — and add() is overridden to throw, so the graph can never be filled in. It also sets own_fields = true at line 19 before calling validate_storage() at line 22: if validation throws, ~IndexHNSW runs and deletes the storage the caller still owns, giving a double free. Neither this constructor nor the non-const pretransform_index()/rabitq_index() overloads have any caller.
There was a problem hiding this comment.
Fixed. The unused pointer constructor and the unused non-const accessors were removed, eliminating both the empty-graph behavior and the exception-path ownership hazard.
|
|
||
| // Private Knowhere serialization tag. Upstream Faiss reserves "IHNr" for | ||
| // its incompatible direct-build/staged-search IndexHNSWRaBitQ format. | ||
| inline constexpr char kHnswRaBitQFourcc[] = "IHRK"; |
There was a problem hiding this comment.
this should not be here. It needs to be in index_read.cpp and index_write.cpp. Yes, let it be duplicate. Also, please make sure that our four-CC codes do not intersect with Faiss ones. So, a given four-CC should not be used by Faiss.
There was a problem hiding this comment.
Fixed. Knowhere HNSW RaBitQ serialization is now implemented in the standard Faiss index_read.cpp/index_write.cpp, while the cppcontrib implementation remains duplicated as requested. The tags are IHRK, IHRC, and IRKC; I checked them against the bundled/upstream Faiss tag set. Cross-reader tests cover standard Faiss and cppcontrib IO.
|
@CLiqing Please create a corresponding PR in the baseline faiss code and we'll ask Faiss guys to take a look. Thanks. |
89f7b5d to
f78e4ac
Compare
|
Created the requested baseline Faiss PR: facebookresearch/faiss#5552 It contains the reusable dense multi-bit RaBitQ layout, serialization, SIMD single/batch-4 scorers, BMI2 runtime fallback, and focused tests. The Knowhere-specific HNSW integration remains in this PR. |
Signed-off-by: ChenLiqing <23721160+CLiqing@users.noreply.github.com>
f78e4ac to
c74c993
Compare
| READ1(hnsw.efConstruction); | ||
| READ1(hnsw.efSearch); | ||
| READ1(hnsw.upper_beam); | ||
| } |
There was a problem hiding this comment.
read_knowhere_HNSW (index_read.cpp:1309-1320) omits the validate_HNSW(hnsw) call that read_HNSW performs at :1306, and the IHRK/IHRC branch at :2552 only calls validate_storage()/validate_cosine_storage(), neither of which inspects hnsw.levels, hnsw.offsets, hnsw.neighbors or hnsw.entry_point. This is the only HNSW-graph-carrying fourcc in read_index_up that bypasses validate_HNSW — the branches at :2611, :3464 and :3488 all go through read_HNSW. Take a valid HNSW_RABITQ index serialized by knowhere (fourcc IHRK), set one entry of neighbors[] to ntotal, and load it through faiss::read_index — reachable from src/index/faiss/faiss.cc:345 (BinarySet bytes) and :365/:368 (file), and exercised directly by tests/ut/test_hnsw_rabitq.cc:323. Deserialization succeeds, then the first search reads out of bounds, because HNSW::neighbor_range (cppcontrib/knowhere/impl/HNSW.cpp:53-58) indexes offsets[]/neighbors[] with no bounds check.
| ../../thirdparty/faiss/tests/test_pq_code_distance.cpp | ||
| ../../thirdparty/faiss/tests/test_cppcontrib_uintreader.cpp | ||
| ../../thirdparty/faiss/tests/test_distances_simd.cpp | ||
| ../../thirdparty/faiss/tests/test_rabitq_simd.cpp |
There was a problem hiding this comment.
tests/faiss/CMakeLists.txt:10 adds test_rabitq_simd.cpp to FAISS_TEST_SRCS, which feeds both the if(__X86_64) target at :38 and the if(__AARCH64) target at :61. The file unconditionally instantiates AVX2/AVX512 explicit specializations (:502, :509, :516, :524-529, :539, :546, :552 are new in this PR) whose definitions live only in rabitq_avx2.cpp/rabitq_avx512.cpp, which cmake/libs/libfaiss.cmake:75 and :124 strip from FAISS_SRCS on aarch64. Building on aarch64 with -DWITH_FAISS_TESTS=ON fails to link faiss_tests with undefined reference to selected_float_sum<(SIMDLevel)1>, multibit::compute_inner_product<(SIMDLevel)2>, multibit::compute_inner_product_batch_4<(SIMDLevel)1>, rearrange_bit_planes<(SIMDLevel)1> and bitwise_and_dot_product_with_popcount<(SIMDLevel)1>. Reproduced on aarch64 with g++ 11.4.0. WITH_FAISS_TESTS defaults to OFF (CMakeLists.txt:42), so CI does not catch it.
| read_index_header(*ixpt, f); | ||
| int nt; | ||
| READ1(nt); | ||
| FAISS_THROW_IF_NOT_MSG( |
There was a problem hiding this comment.
The new IRKC branch at index_read.cpp:2385-2392 reads the transform count with only an nt >= 0 check, while the IxPT branch it was copied from adds FAISS_CHECK_DESERIALIZATION_LOOP_LIMIT at :2410 and validates chain[0]->d_in == d, chain[i]->d_in == chain[i-1]->d_out and chain[nt-1]->d_out == index->d at :2421-2443. validate_norms() at :2394 only checks inverse-norm count and positivity, so a top-level IRKC index gets no chain validation at all. Load a crafted IRKC index whose single chain transform has d_in != d through faiss::read_index (reachable from src/index/faiss/faiss.cc:345). Deserialization succeeds, then IndexPreTransform::apply_chain transforms with mismatched dimensions during search. A large nt additionally drives unbounded chain growth that the loop-limit macro — used 13 times elsewhere in this same file — exists to bound.
issue: #1747
Summary
HNSW_RABITQwith 1-8 bit dense database codes and FP32 queries (qb=0), supporting L2, inner product, and cosine similarityRandomRotationMatrix -> IndexRaBitQstorage for searchCompatibility and scope
HNSW_RABITQV1 is immutable after its initial build and supports L2/IP/COSINErbq_bits_queryis rejected explicitlyIndexRaBitQretains its existing 1-9 bit rangefaiss::cppcontrib::knowhereand usesIHRK/IHRC, whileIHNrremains reserved for #5526; shared bundled-Faiss quantizer/SIMD changes will still require normal reconciliation during future Faiss updates